Skip to content

bootstrap: fix clean's remove_dir_all implementation #129187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 22, 2024
Merged

bootstrap: fix clean's remove_dir_all implementation #129187

merged 1 commit into from
Aug 22, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Aug 17, 2024 •
edited
Loading

It turns out bootstrap's clean.rs's hand-rolled rm_rf (which probably comes before std::fs::remove_dir_all was stable) is very broken on native Windows around both read-only files/directories and especially symbolic links. So instead of rolling our own, just use std::fs::remove_dir_all.

This is a related to compiletest's own rm_rf implementation #129155 which happens to be also buggy, which in turn is a blocker for the rmake.rs test port #128562 that heavily exercises symlinks (I was reviewing #128562 and testing it on native Windows which is how I found out).

Fixes #112544 (because now we handle Windows symlinks properly).

try-job: x86_64-msvc
try-job: i686-mingw
try-job: test-various
try-job: armhf-gnu
try-job: aarch64-apple
try-job: aarch64-gnu

@jieyouxu jieyouxu added the O-windows Operating system: Windows label Aug 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2024

r? @Kobzol

rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2024

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 17, 2024
@jieyouxu

This comment was marked as outdated.

Sign in to view
@bors

This comment was marked as outdated.

Sign in to view
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 17, 2024
@bors
Auto merge of rust-lang#129187 - jieyouxu:squeaky-clean-windows-symli…
e73eaeb 
…nks, r=<try>

bootstrap: fix clean's remove_dir_all implementation

It turns out bootstrap's `clean.rs`'s hand-rolled `rm_rf` (which probably comes before `std::fs::remove_dir_all` was stable) is very broken on native Windows around both read-only files/directories and especially symbolic links. So instead of rolling our own, just use `std::fs::remove_dir_all`. Because I wasn't super sure about `std::fs::remove_dir_all`'s behavior and to catch `std::fs::remove_dir_all`'s behavioral changes here on forward, I added a collection of tests that checks if our expectation of the behavior of `std::fs::remove_dir_all` and its underlying Unix syscalls and Win32 APIs matches with its actual behavior.

This is a blocker for compiletest's own `rm_rf` implementation rust-lang#129155 which happens to be also buggy, which in turn is a blocker for the rmake.rs test port rust-lang#128562 that heavily exercises symlinks (I was reviewing rust-lang#128562 and testing it on native Windows which is how I found out).

I also left a FIXME for `detect_src_and_out` due to a failing assertion on native Windows:

```
---- core::config::tests::detect_src_and_out stdout ----
thread 'core::config::tests::detect_src_and_out' panicked at src\core\config\tests.rs:72:13:
assertion `left == right` failed
  left: "E:\\tmp"
 right: "C:\\tmp"
```

Fixes rust-lang#112544 (because now we handle Windows symlinks properly).

try-job: x86_64-msvc
try-job: i686-mingw
try-job: test-various
try-job: armhf-gnu
try-job: aarch64-apple
try-job: aarch64-gnu
@jieyouxu jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ChrisDenton
Copy link
Member

I feel like the std::fs::remove_dir_all tests should be std tests rather than bootstrap tests.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment was marked as outdated.

Sign in to view
@jieyouxu
Copy link
Member Author

jieyouxu commented Aug 17, 2024 •
edited
Loading

I feel like the std::fs::remove_dir_all tests should be std tests rather than bootstrap tests.

I might actually remove the tests because std already has some for remove_dir_all. The set of tests here make simplifying assumptions which would be subtly wrong for std (fallback behavior for Windows targets less than Windows 10 RS1 are not accounted for in this set of tests).

@jieyouxu jieyouxu force-pushed the squeaky-clean-windows-symlinks branch from e736cb4 to 1ef814a Compare August 17, 2024 10:50
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jieyouxu jieyouxu force-pushed the squeaky-clean-windows-symlinks branch from 1ef814a to c762aa6 Compare August 17, 2024 11:06
@jieyouxu jieyouxu force-pushed the squeaky-clean-windows-symlinks branch from c762aa6 to 8af5c0a Compare August 17, 2024 11:27
@jieyouxu
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 17, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Aug 17, 2024

@bors try

@bors
Copy link
Collaborator

bors commented Aug 17, 2024

⌛ Trying commit 8af5c0a with merge ca401c6...

@bors
Copy link
Collaborator

bors commented Aug 17, 2024

☀️ Try build successful - checks-actions
Build commit: ca401c6 (ca401c6512833e829cbb2f14741b81fb9fff8d0a)

@bors
Copy link
Collaborator

bors commented Aug 20, 2024

📌 Commit 1687c55 has been approved by Kobzol

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2024
@tgross35 tgross35 mentioned this pull request Aug 21, 2024
Closed
@tgross35 tgross35 mentioned this pull request Aug 21, 2024
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 21, 2024
Merged
@jieyouxu jieyouxu mentioned this pull request Aug 22, 2024
Closed
@bors bors merged commit 9fd8a2c into rust-lang:master Aug 22, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 22, 2024
@jieyouxu jieyouxu deleted the squeaky-clean-windows-symlinks branch August 22, 2024 09:28
@krasimirgg
Copy link
Contributor

For some reason, after this we're getting some failures with x.py clean. Initially I saw this in our rust+llvm@head build bot: https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/30548#01917a4a-a850-4dd1-9c7a-38da213bc8ea.

I tried it with a freshly-cloned version of rust and was able to reproduce as:

git clone https://github.com/rust-lang/rust
cd rust
git clean -fdx
python3 x.py clean --all
...
thread 'main' panicked at src/core/build_steps/clean.rs:178:9:
failed to `remove_dir_all` at `tmp`: No such file or directory (os error 2)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Creating a tmp directory under the checked out rust seems to fix it.

@ChrisDenton
Copy link
Member

ChrisDenton commented Aug 22, 2024 •
edited
Loading

Ah, I believe that's fixed in std by #127623

In the meantime bootstrap will need a workaround that skips deleting directories that don't exist.

EDIT: Oh, maybe not fixed as it would still error on the top-level directory. So the workaround would need to be permanent.

@jieyouxu
Copy link
Member Author

Oh wait. I think I didn't consider the top-level directory missing but was ultrafocused on read-only/symlinks of nested directory entries.

I'm going to revert the two PRs then reland the PRs properly accounting for missing top-level directories.

@jieyouxu
Copy link
Member Author

For some reason, after this we're getting some failures with x.py clean. Initially I saw this in our rust+llvm@head build bot: buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/30548#01917a4a-a850-4dd1-9c7a-38da213bc8ea.

Thanks for the heads up! This PR and the compiletest PR is indeed the culprit. My apologies for borking the buildsystem.

@jieyouxu jieyouxu mentioned this pull request Aug 22, 2024
Merged
@jieyouxu
Copy link
Member Author

For some reason, after this we're getting some failures with x.py clean.

Revert PR is up at #129413.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kobzol Kobzol Kobzol left review comments

Assignees

@Kobzol Kobzol

Labels
O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.82.0
Development

Successfully merging this pull request may close these issues.

Bootstrap cannot remove host symlink on Windows if created from WSL2
7 participants
@jieyouxu @rustbot @bors @rust-log-analyzer @ChrisDenton @Kobzol @krasimirgg